Another attempt at an OIDC integration#1568
Another attempt at an OIDC integration#1568KJTsanaktsidis wants to merge 22 commits intoNixOS:masterfrom
Conversation
I use Kanidm, which does not have support for custom roles / claims. That makes this approach DOA for me. |
|
What can Kanidm do/what would you expect the role mapping configuration on the Hydra side to look like? |
|
I’ll have a play around with this when I get a chance, but it looks like kanidm can map its groups to custom claims with this mechanism - https://kanidm.github.io/kanidm/stable/integrations/oauth2/custom_claims.html sidebar, but kanidm looks like it might be a lot less painful to integrate into the test/development environment than keycloak so I might try swapping it |
|
for context, my hydra instance with @ners patch use kanidm as well and it looks like this |
|
Hm, OK, so the reason kanidm can't literally just put the inside-hydra role names in a custom claim is because the custom claim values have to match the regex Most of the hydra roles (all except This limitation is probably a bit too strict on the kanidm side IMO - it's been discussed a few times kanidm/kanidm#2641, kanidm/kanidm#2882. I'll go add my two-cents worth into there. And in the meanwhile, I guess, make the hydra side accept e.g. |
f9f665d to
7075f60
Compare
|
OK I gritted my teeth and managed to write an end-to-end test for the OIDC integration (against a temporary Kanidm |
7075f60 to
5b86036
Compare
|
Note also the work in the lix fork of hydra: https://git.lix.systems/lix-project/hydra/pulls/73 |
|
Oh interesting
this is probably worth doing here too
this also sounds worth doing (or, at least reimplementing these specia cases in terms of the generic OIDC support) but probably not in a first pass - need to think carefully about the migration strategy to avoid breaking existing deployments |
|
@KJTsanaktsidis sorry, just see this just now. I will put it on my list. |
This will be of use when developing & testing OIDC
The OIDC work needs to cache some information (at least, the JWKS keys) so we don't ask for them over and over again needlessly. An in-process cache is in theory fine, but using Plugin::Cache nicely gives us expiry support which we need too (to catch key revocations).
The login code in doEmailLogin is treating email address & username as the same concept, but it shouldn't be. Allow passing in a username field explicitly to doEmailLogin to set as the user's username column. The templates have also been changed to call a new usernameForDisplay method which will be able to customise the display of the username in circumstances where the username would look stupid (e.g. in the forthcoming OIDC support)
OIDC providers are configured in the config file under <oidc> <provider foobar>. There can be multiple providers, each one will get a menu item in the dropdown to log in with. After login is successful, we validate all the claims and then perform doEmailLogin, creating the user if it did not exist.
If presented with a "hydra_roles" claim, we will set the user roles to that claim.
5b86036 to
27d5d98
Compare
|
I rebased this for you. |
Without RP-Initiated Logout, users stay signed in at the IdP after logging out of Hydra, so clicking "Sign in" immediately re-authenticates without a password prompt. The endpoint is read from the discovery document or can be set manually. Adapted from https://git.lix.systems/lix-project/hydra/pulls/73 Co-authored-by: Jörg Thalheim <joerg@thalheim.io>
fixup! Implement OIDC login Sanitize 'after' parameter to prevent open redirect: //evil.com would be interpreted as a protocol-relative URL by browsers.
fixup! Implement OIDC login Use constant-time comparison for state and nonce to avoid timing side-channels.
fixup! Implement OIDC login - Reject token endpoint responses with HTTP error but no 'error' field - Validate aud claim exists before dereferencing
fixup! Add mapping of OIDC claims to roles Wrap setRoles INSERT+DELETE in a transaction so role updates are atomic.
fixup! Implement OIDC login Don't let an unreachable IdP prevent Hydra from starting. Log the error and disable OIDC login instead of dying in setup_finalize.
fixup! Add test for OIDC implementation Move test-only Perl modules (TestWWWMechanize, HTTPCookieJar, ...) from the main hydra package to hydra-tests where they belong.
fixup! hydra-server: redirect to OIDC end_session_endpoint on logout Add CSRF protection to GET /logout: require a token derived from the session ID (sha256(sessionid + ':logout')) so cross-site requests cannot log the user out. The token is exposed to templates via the stash and embedded in the Sign out link.
|
@KJTsanaktsidis I found a few issues with the implementation. Please have a look. |
| ); | ||
|
|
||
|
|
||
| # Derive a CSRF token for GET /logout from the session ID. Using a hash |
There was a problem hiding this comment.
We are technically not doing this anywhere else yet, but I thought we could start at least. Hydra can also serve html from users, so this sounds like a bit of a dangerous combination.
|
I pushed a stack of
|
fixup! Implement OIDC login Remove unused imports (Data::Dumper debug leftover, sha256_hex).
fixup! Implement OIDC login Drop redundant base64URLEncode wrapper: MIME::Base64::encode_base64url already strips '=' padding per RFC 4648 §5, so the s/=+$//r was a no-op.
fixup! Implement OIDC login Collapse 4x repeated config validation into a loop. Bonus: the error now names which field is missing instead of listing all four.
fixup! Implement OIDC login Let Crypt::JWT verify iss/aud/exp/nbf instead of reimplementing the OIDC §3.1.3.7 checklist by hand. The library already handles the array-or-scalar aud case and rejects alg=none. Keep only the OIDC-specific nonce check and sub presence check.
fixup! Implement OIDC login Drop Crypt::URandom::Token dependency (and its flake overlay). base64url-encoding raw urandom bytes satisfies both RFC 6749 state/nonce and RFC 7636 PKCE verifier requirements without needing a custom token-from-charset generator.
fixup! Implement OIDC login Modernize with Perl 5.36+ features: - Subroutine signatures replace 'my ($self, ...) = @_' boilerplate - try/catch replaces eval/$@ (avoids the $@-clobbered-by-destructor footgun and gives a lexically-scoped error variable) - Simplify scope construction with join+grep
fixup! Implement OIDC login Make OIDC discovery lazy instead of blocking startup. Previously resolveOIDCConfig did an HTTP GET to the discovery_url in setup_finalize, meaning every Hydra start blocked on the IdP for up to 10s and an IdP that was down at boot left OIDC broken until restart. Now resolveOIDCConfig only loads secrets from disk and validates static config (no network). Discovery happens in _resolvedConf on the first login attempt, cached for 1h via Plugin::Cache and also written back into the in-process config so subsequent calls short-circuit. An IdP outage now only affects users trying to log in (503), and recovers automatically without a restart.
Building on the work of @lheckemann in #1298 & in @ners in https://github.com/ners/hydra/tree/oidc, I took a stab at trying to shape that work up in a way that can get merged.
This implementation:
discovery_url(although you can specifyissuer,authorization_endpoint,token_endpoint, andjwks_urlmanually)hydra_rolesclaim, and, if present, sets roles on the user based on that. I did think about implementing some kind of mapping mechanism, but Keycloak, at least, seems to make it pretty easy to beat the mapping into shape on that side, so perhaps there is no need.I banged my head trying to write some good tests for this, but really what this needs to meaningfully test things are full-system integration tests. I have a working
KeycloakContext.pmon my machine which starts a Keycloak instance in the test framework, but I'm not really sure how to write the kind of browser test that would be needed to go through the login flow. Any suggestions?